Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 124-128: Update the guidance to exempt intentional Firebase
emulator mock casts by noting that "as any" in test files under the
functions/src/__tests__ tree (e.g., functions/src/__tests__/rules/*.test.ts,
functions/src/__tests__/callables/close-report.test.ts,
functions/src/__tests__/acceptance/phase-4a-acceptance.test.ts) are allowed for
emulator compatibility and should not be counted as tech-debt; keep the existing
action to replace other `any` usages with proper Firestore types or `unknown`,
but add a clear exception sentence and pattern-based exclusion for the
emulator-related test files.
- Around line 29-33: The refactor-audit doc incorrectly lists bare `catch {}`
blocks that were already fixed; update the inventory to reflect current code by
removing or correcting the entries that reference the now-converted bare catches
and any stale line ranges, and refresh the “Action”/“Effort” items accordingly;
specifically, remove the stale bare-catch mentions and adjust the suggested
extractions to match the current components/hooks (GpsButton,
MunicipalitySelector, BarangaySelector, ContactFields and hooks useGpsLocation,
useMunicipalityBarangays), ensuring the document only calls out real remaining
issues and accurate locations.
- Around line 4-18: The P0 section in docs/refactor-audit-2026-04-23.md is out
of date: it still lists two failing tests in shared-sms-parser even though this
PR includes the fixes; update the "Health Check" and "## 🔴 P0 — Fix Before
Anything Else" content to reflect that packages/shared-sms-parser tests now pass
(remove or mark resolved the two failing items about inbound.test.ts and the two
specific failures) and adjust the status indicators (Lint/Typecheck/Tests) so
the audit baseline matches the current branch state.

In `@functions/src/__tests__/callables/https-error.test.ts`:
- Around line 11-17: The test currently iterates
Object.keys(BANTAYOG_TO_HTTPS_CODE) which only checks the map's existing keys
and can miss unmapped enum entries; change the loop to iterate the actual
BantayogErrorCode enum values (e.g., use Object.values(BantayogErrorCode) cast
to BantayogErrorCode[]) and for each enum value assert
BANTAYOG_TO_HTTPS_CODE[value] is defined and typeof === 'string' so every enum
member is validated against the BANTAYOG_TO_HTTPS_CODE mapping.

In `@functions/src/http/sms-inbound.ts`:
- Around line 96-103: The WARNING log in the catch block for msisdn parsing
currently includes String(err), which may leak PII; update the catch handling
around msisdnHash and the log(...) invocation to avoid logging raw error
text—replace String(err) with a sanitized token such as error.name and/or
error.code (e.g., err instanceof Error ? err.name : 'UnknownError') or a fixed
safe message, or redact digits from the error string before logging; ensure you
modify the catch block that computes msisdnHash and calls log({ severity:
'WARNING', code: 'msisdn.invalid', ... }) so only non-PII error identifiers are
included.

In `@functions/src/services/fcm-send.ts`:
- Around line 69-72: The catch block in fcm-send.ts currently pushes String(err)
into the warnings array, mixing raw transport text with machine-readable codes;
change the catch to only push stable warning codes (e.g., keep
'fcm_network_error' or add another pre-defined code) and remove the String(err)
push so the returned warnings remain machine-readable. Instead of returning the
raw error, log/record the full error to your internal logger or monitoring (call
logger.error(err, { context: 'fcm-send' }) or send to Sentry) from inside the
same catch block so you preserve diagnostics without exposing internal details
to clients. Ensure the variable names referenced are warnings and the catch in
the sendFcm/fcm-send handler are updated accordingly and the API response only
contains the sanitized codes.

In `@functions/src/services/sms-providers/semaphore.ts`:
- Around line 62-66: The catch block that currently throws
SmsProviderRetryableError for unparseable responses needs to treat 4xx responses
as non-retryable: if res.status is in the 400–499 range, throw the non-retryable
error type (e.g., SmsProviderPermanentError or the project's equivalent) with a
clear message including res.status and the parse error; only construct
SmsProviderRetryableError for server-side/5xx errors (and network-like
failures). Update the logic around SmsProviderRetryableError creation in the
catch to branch on res.status (>=500 = retryable/provider_error, 400–499 =
non-retryable/invalid_request) and include the original error details in the
message.

In `@packages/shared-sms-parser/src/inbound.ts`:
- Around line 45-48: The current catch in inbound.ts swallows every error when
trying to load '@bantayog/shared-data', returning FALLBACK_BARANGAYS even for
real runtime/data errors; update the catch around the dynamic import/load so it
only suppresses expected module-not-found errors (e.g., check (err as
NodeJS.ErrnoException).code === 'MODULE_NOT_FOUND' or inspect the error message
for the package name or ERR_MODULE_NOT_FOUND) and rethrow or propagate any other
errors so real failures surface; keep the fallback behavior for the specific
missing-module case and reference FALLBACK_BARANGAYS and the
'@bantayog/shared-data' load location when making the change.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/shared-validators/src/idempotency.ts`:
- Around line 21-29: The current guard only checks typeof subtle === 'object'
which lets null through; update the check around the Web Crypto usage in
idempotency.ts to validate that globalThis.crypto.subtle is present and usable
before calling subtle.digest: explicitly verify !subtle || typeof subtle.digest
!== 'function' (or equivalent truthy check) and throw the original descriptive
Error if the guard fails; ensure this check sits immediately before using
canonicalize, JSON.stringify, TextEncoder, and subtle.digest so you never call
subtle.digest on null/undefined.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/shared-validators/lib/idempotency.js`:
- Around line 18-21: The runtime check for the Web Crypto Subtle API using
"const subtle = globalThis.crypto?.subtle;" doesn't handle null (typeof null ===
"object"), so update the guard in idempotency.js to explicitly check for
null/undefined and presence of the digest method before assuming subtle is
usable; e.g., validate "subtle" is truthy and has a "digest" function (check
subtle && typeof subtle.digest === 'function') and log/throw the original
clearer error path if missing so subsequent use of subtle.digest won't hit
"Cannot read property 'digest' of null".

---

Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 138-140: The roadmap lists resolved tasks as pending — remove the
completed entries for "shared-sms-parser" (P0 fix) and the "inbound.ts"
bare-catch fix from the execution order so the schedule only shows outstanding
work; instead mark those items as completed or archived and ensure entries
referencing Step2WhoWhere.tsx, shared-sms-parser, and inbound.ts are updated to
reflect status (e.g., "Done" or linked to the completing PR) and regenerate the
execution order so it no longer includes resolved tasks.
- Around line 79-99: The audit's "Inconsistent error handling (6+ catch
patterns)" section contains stale data: it still lists
functions/src/services/sms-providers/semaphore.ts as a bare-catch site and the
total bare-catch count (13) is outdated — recalculate the current catch-pattern
counts across the branch and update the summary counts and the file list in that
section (the heading "Inconsistent error handling (6+ catch patterns)" and its
bullet list). Ensure you run the same detection script/regex used originally,
verify intentional exceptions (e.g., MODULE_NOT_FOUND or retry logic) remain
annotated, remove entries already fixed, and replace the old numbers and file
entries with the new, accurate values.
- Around line 30-37: The docs entry for
packages/shared-sms-parser/src/inbound.ts incorrectly reports a bare `catch {}`;
update the `inbound.ts` debt note to reflect that the code now uses `catch (err:
unknown)` with selective fallback/rethrow behavior (remove the stale "bare
catch" claim and the duplicate_comment marker). Locate the `inbound.ts` entry in
the audit doc and replace the bare-catch assertion with a brief description of
the current catch handling (mentioning `catch (err: unknown)` and its selective
fallback/rethrow semantics) and remove any duplicate references.

In `@packages/shared-sms-parser/src/inbound.ts`:
- Around line 45-55: The catch currently treats any MODULE_NOT_FOUND inside the
try as a harmless missing package and returns FALLBACK_BARANGAYS; instead, only
swallow the error when the missing module is specifically
'@bantayog/shared-data'. Change the predicate around isModuleNotFound to also
check the error message (or error.path/name) contains the literal
'@bantayog/shared-data' (while preserving the existing code check for err.code
=== 'MODULE_NOT_FOUND'); if both conditions match return FALLBACK_BARANGAYS,
otherwise rethrow the error so real runtime failures in functions called inside
the try are not silently suppressed. Use the same error variable and keep the
throw err behavior for non-matching cases.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 380-390: The catch block in Step2WhoWhere.tsx currently swallows
all errors except quota and security cases; update it to log unexpected storage
errors instead of silently discarding them by adding a console.error (or app
logger) for errors that are not DOMException quota/security cases—keep the
existing DOMException checks (err instanceof DOMException and err.name ===
'QuotaExceededError' || err.code === 22) and the intentional silent handling for
SecurityError, but for any other err value log a clear message (e.g.,
"[Step2WhoWhere] unexpected storage error") along with the error object; apply
the same change to the other catch block around the 431-440 region.

In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 26-39: The probe timeout (timeoutId) is only cleared on the
success path, leaving the timer running on errors; update the probe logic inside
useOnlineStatus (the fetch to PROBE_URL using controller.signal and
PROBE_TIMEOUT_MS) so that clearTimeout(timeoutId) is always executed (e.g., move
it into a finally block or call it in the catch before setting
setProbeOnline(false)), ensuring the AbortController (controller) and timeout
are cleaned up regardless of success or failure.

In `@packages/shared-validators/src/idempotency.test.ts`:
- Around line 63-98: Add a regression test that exercises the WeakSet-based
cycle detection by passing a self-referential object or array into
canonicalPayloadHash so the canonicalize circular-reference branch runs; for
example create const obj: any = {}; obj.self = obj; (or a self-referential
array) and then assert await expect(canonicalPayloadHash(obj)).rejects.toThrow()
(or rejects/toThrow matching your error) inside the same test style as the other
Web Crypto guards, cleaning up globals if used; this ensures the canonicalize
cycle-detection path is covered and locked in.

In `@packages/shared-validators/src/idempotency.ts`:
- Around line 31-39: The catch block around subtle.digest in idempotency.ts
currently swallows the original error; update the try/catch in the function that
computes the SHA-256 digest so the catch captures the thrown error (e.g., "err")
and rethrows or wraps it including the original error message/cause in the new
Error passed from that block, preserving provider/runtime details instead of
discarding them; reference the existing subtle.digest call and the surrounding
error thrown ('Web Crypto API (globalThis.crypto.subtle.digest) failed...') to
include the underlying error information in that message.

In `@packages/shared-validators/src/msisdn.test.ts`:
- Around line 67-78: Add a runtime non-string salt test in
packages/shared-validators/src/msisdn.test.ts to ensure hashMsisdn enforces its
boundary: call hashMsisdn('+639171234567', null), hashMsisdn('+639171234567',
undefined) and hashMsisdn('+639171234567', 12345) (or at least one
representative non-string) and expect the call to throw (matching the existing
error expectation such as 'at least 16 characters' or a TypeError) so the
function's guard against non-string salts is locked down; update the test block
alongside the existing empty/short salt cases referencing the hashMsisdn symbol.

In `@packages/shared-validators/src/msisdn.ts`:
- Around line 50-54: The current validation in hashMsisdn reads salt.length even
when salt isn't a string which can throw; update the check to avoid
dereferencing non-strings by first determining if typeof salt === 'string' and
only then reading length, and build the Error message using a safe value (e.g.
the actual string length when salt is a string or a descriptive token like
"invalid" / the typeof value otherwise). Ensure the condition still enforces
"salt must be a string of at least 16 characters" and reference the salt
parameter and the hashMsisdn validation block when making the change.

---

Outside diff comments:
In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 43-44: The comment above the probing logic in useOnlineStatus is
outdated (mentions “every 30s” and setTimeout) — update it to accurately
describe the current behavior: it probes immediately once and then schedules
recurring probes every 10s using setInterval; mention the immediate call and
that setInterval is used to avoid calling setState synchronously within the
effect body so readers understand the scheduling approach used in the
useOnlineStatus hook.

---

Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 86-90: Update the summary and the file list so they agree: either
change the count "0 in production source" to reflect the two production files
listed (functions/src/services/fcm-send.ts and
functions/src/triggers/inbox-reconciliation-sweep.ts) or remove those two files
from the "remaining" list if they are intentionally excluded; also clarify why
the two listed occurrences are not counted as production (e.g., mark them as
exceptions with a short parenthetical like "(intentional retry logic)" for
fcm-send and "(intentional transaction contention skip)" for
inbox-reconciliation-sweep) and make the header count match the adjusted list
and explanations.
- Line 24: Update the documentation line to match the implementation's catch
signature: replace the note that says `catch (_err: unknown)` with `catch (err:
unknown)` so it reflects the actual code in Step2WhoWhere.tsx where the catch
uses `catch (err: unknown)`; ensure the wording exactly matches that symbol
usage to avoid audit drift.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 380-391: Extract the duplicated DOMException branching into a
small helper (e.g., isQuotaExceededError(err: unknown) and isSecurityError(err:
unknown) or a single classifyStorageError returning
{quota:boolean,security:boolean}) and replace the duplicate try/catch branches
in Step2WhoWhere's storage read/write handlers with calls to that helper;
implement the helper to check err instanceof DOMException and the deprecated
code check (err.name === 'QuotaExceededError' || err.code === 22) for quota and
err.name === 'SecurityError' for security, then keep the existing logging
behavior in the catch blocks but delegate decision logic to the new helper so
both the read and write catch sites use the same classification logic.

In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 4-5: The probe timing values in useOnlineStatus.ts
(PROBE_TIMEOUT_MS and PROBE_INTERVAL_MS) are too short and may cause
online/offline flapping that triggers retries in useSubmissionMachine; increase
PROBE_TIMEOUT_MS (e.g., to 5000 ms) and PROBE_INTERVAL_MS (e.g., to 30000 ms) or
make them configurable/backoff-aware so transient mobile network hiccups don't
rapidly flip isOnline and cause retry thrashing. Ensure updates are applied
where PROBE_TIMEOUT_MS and PROBE_INTERVAL_MS are referenced so the submission
machine sees the calmer probe cadence.

In `@docs/refactor-audit-2026-04-23.md`:
- Around line 19-24: The documentation incorrectly states "707-line component"
for Step2WhoWhere.tsx; update the wording to say the file is ~707 lines while
the Step2WhoWhere component body is much smaller and the file contains large
constants and helpers. Edit the entry referencing
apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx and change
"707-line component" to something like "≈707-line file (component body is much
smaller; large constants/helpers inflate file size)" and ensure the item
references the component name Step2WhoWhere to keep clarity.
- Around line 52-53: The section title "Entire apps/packages have zero tests" is
inaccurate relative to the table that shows apps like `apps/responder-app`
having 1 test; update the heading text to match the table values (e.g., "Entire
apps/packages have few or no tests" or "Apps/packages with zero or few tests")
and ensure any related summary lines reference `apps/responder-app` correctly so
the title and table are consistent.

---

Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 86-90: Update the contradictory counts for bare "catch {}" in
docs/refactor-audit-2026-04-23.md so the summary on line 86 matches the later
summary around line 154: verify the actual production occurrences (files listed:
functions/src/services/fcm-send.ts,
functions/src/triggers/inbox-reconciliation-sweep.ts) and either change "2 in
production source" to the correct number or update the later summary to reflect
"2 in production source"; also ensure the "Files with bare `catch {}`" list and
the parenthetical notes for those files remain accurate and consistent with the
corrected total.